Add license sniff and fixes #2

geerteltink · 2017-04-12T15:10:20Z

The test are executed independent of the standard AbstractSniffUnitTest class. That class is only available if PHP_CodeSniffer is downloaded by composer with prefer-source.

Copyright year detection:

Detect current copyright year from LICENSE.md and/or COPYRIGHT.md
Detect current copyright year from all source files
Globally exclude /TestAssets/

Violation detection (when running phpcs):

Check file-level docblock
Check missing/invalid see tag
Check missing/invalid copyright tag
Check missing/invalid license tag
Check order of see, copyright and license tags
Check for current copyright year in LICENSE.md and COPYRIGHT.md
Check text in LICENSE.md and COPYRIGHT.md to make sure it's consistence in all components

Fixes (when running phpcbf):

Create missing LICENSE.md and/or COPYRIGHT.md
Use the detected copyright year in LICENSE.md and COPYRIGHT.md
Fix invalid see tag
Fix invalid copyright tag
Fix invalid license tag
Update copyright in LICENSE.md and COPYRIGHT.md to the current year
Update the entire text in LICENSE.md and COPYRIGHT.md for consistency
Fix missing newline after comment

TODO:

Discussion:

Is the text for COPYRIGHT.md correct?
Is the text for LICENSE.md correct?
How to include the ruleset in components?
1. <rule ref="vendor/zendframework/zend-coding-standard/ruleset.xml"/>
2. <rule ref="vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml"/>
  The first is like a wrapper and loads the second one anyway.

@see

The test are executed independent of the standard AbstractSniffUnitTest class. That class is only available if PHP_CodeSniffer is downloaded by composer with prefer-source. Copyright year detection: - Detect current copyright year from LICENSE.md and/or COPYING.md - Detect current copyright year from all source files Violation detection (when running phpcs): - Check file-level docblock - Check missing/invalid @see tag - Check missing/invalid @copyright tag - Check missing/invalid @license tag - Check order of @see, @copyright and @license tags Fixes (when running phpcbf): - Create missing LICENSE.md and/or COPYING.md - Use the detected copyright year in LICENSE.md and COPYING.md - Fix invalid @see tag - Fix invalid @copyright tag - Fix invalid @license tag

weierophinney

Mostly some consistency/formatting/docblock changes requested. The bigger question/issue is with the location of the ruleset.xml file.

weierophinney · 2017-04-12T16:19:06Z

.travis.yml

+
+matrix:
+  include:
+    - php: 7


Maybe should also test it on 7.1? That way we'll know if there's breakage if/when we want to use it against that version instead of 7.0.

weierophinney · 2017-04-12T16:19:36Z

.travis.yml

+  - if [[ $TEST_COVERAGE != 'true' ]]; then phpenv config-rm xdebug.ini || return 0 ; fi
+
+install:
+  - travis_retry composer install $COMPOSER_ARGS --ignore-platform-reqs


Why --ignore-platform-reqs here? Is there a compelling reason?

I've added the lowest/locked/latest strategy.

weierophinney · 2017-04-12T16:26:24Z

ruleset.xml

-            <property name="ignoreBlankLines" value="false"/>
-        </properties>
-    </rule>
+    <rule ref="vendor/zendframework/zend-coding-standard/ZendCodingStandard/ruleset.xml"/>


A little confused by the changes here.

In components that already use this package, this particular file is the one referenced. As an example, from zendframework/zend-expressive:

<rule ref="./vendor/zendframework/zend-coding-standard/ruleset.xml"/>

Shouldn't this then be the full ruleset, and not reference another ruleset?

The referenced path would be potentially valid for components referencing the ruleset installed via Composer, assuming that phpcs follows the reference, but is malformed; ZendCodingStandard is a subdirectory of src/~

The referenced path is invalid for this repository (should be src/ZendCodingStandard/ruleset.xml), though I'm not sure if that's a problem or not.

I suspect the testing against other components will tell us for sure if this works as expected, but thought I'd highlight the potential issues first.

In components that already use this package, this particular file is the one referenced. As an example, from zendframework/zend-expressive... Shouldn't this then be the full ruleset, and not reference another ruleset?

As you say, in expressive, this file is imported: vendor/zendframework/zend-coding-standard/ruleset.xml.
However for PHP_CodeSniffer to work, a ruleset.xml file must be some/path/<ruleset_name>/ruleset.xml. I've tried several options and even including this in src directly. This is the only way PHP_CodeSniffer detects the ruleset as a valid ruleset. I also tried this with PHP_CodeSniffer 3.0RC with the same result.

I guess using twice the name ruleset.xml is confusing. The first (this) one includes the real standard ruleset. I guess including './vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml' might also work, but than all components need another PR.

The referenced path is invalid for this repository (should be src/ZendCodingStandard/ruleset.xml), though I'm not sure if that's a problem or not.

Since this ruleset.xml file is only meant for inclusion in other projects, it doesn't matter. Running phpcs inside this project will use phpcs.xml.

I have tested this against another repo and it worked as it is. I'll test zf components later on.

@weierophinney There was actually an error in that file. the correct location would be <rule ref="vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml"/>.

weierophinney · 2017-04-12T16:27:15Z

src/ZendCodingStandard/Sniffs/Commenting/FileLevelDocBlockSniff.php

+
+    private $copyrightChanged;
+
+    private $fix;


The above should have @var annotations declaring types.

weierophinney · 2017-04-12T16:27:40Z

src/ZendCodingStandard/Sniffs/Commenting/FileLevelDocBlockSniff.php

+            true
+        );
+
+        $ignore = [


Maybe this declaration should be as a constant or class property?

weierophinney · 2017-04-12T16:33:31Z

test/SniffTestCase.php

+{
+    public function processAsset($asset)
+    {
+        $phpcs = new \PHP_CodeSniffer();


Import this class.

weierophinney · 2017-04-12T16:33:46Z

test/SniffTestCase.php

+            'showSources' => true,
+        ]);
+
+        $reflection = new \ReflectionProperty($phpcs->cli, 'values');


Import this class.

weierophinney · 2017-04-12T16:34:01Z

test/SniffTestCase.php

+        return $phpcs->processFile($asset);
+    }
+
+    public function assertErrorCount($expectedCount, \PHP_CodeSniffer_File $file)


Import this class

weierophinney · 2017-04-12T16:34:49Z

test/SniffTestCase.php

+
+    public function assertErrorCount($expectedCount, \PHP_CodeSniffer_File $file)
+    {
+        if (! \is_int($expectedCount)) {


Why is this globally qualified?

I actually have no issues with globally qualifying functions and constants. However, choose one or the other, and stick with it throughout. This one jumped at me because I haven't seen it done anywhere else.

This one jumped at me because I haven't seen it done anywhere else.

Me neither. I copied a basic function from phpunit as an example and didn't notice this.

weierophinney · 2017-04-12T16:36:23Z

test/Sniffs/Commenting/FileLevelDocBlockSniffTest.php

+    public function assetsProvider()
+    {
+        return [
+            'FileLevelDocBlockPass'                   => [


Don't pad the => declaration here or with siblings to this item. Since it's a multi-line array declaration, alignment with other items of the same nesting level isn't necessary, and actually distracts.

@weierophinney

- Add php 5.6 and 7.1 to the matrix - Use lowest/locked/latest strategy - Enable notifications for IRC and Slack - Generate and upload code coverage per @weierophinney

@weierophinney

per @weierophinney

geerteltink · 2017-04-12T22:43:59Z

@weierophinney There is an inconsistency in the copyright line in the license files throughout the components:

Copyright (c) 2015-2016, Zend Technologies USA, Inc. All rights reserved.

Vs

Copyright (c) 2015-2016, Zend Technologies USA, Inc.

All rights reserved.

Also I've tested this against zend-expressive and all source code files are updated as expected. I've only found some issues with updating the copyright date which needs to be fixed.

The copyright dates where not correctly updated and were also out of sync in some cases. If a valid copyright date range is detected in a source code file, both files are now checked and updated if needed. Also before one of the files is updated, the other file is checked as well to calculate the correct date range and make sure they are in sync.

geerteltink · 2017-04-13T19:01:50Z

@weierophinney I've ironed out the issues. The dates in COPYING and LICENSE are now updated correctly. And the text in those files are now forced to be the same as the default so we get consistency between all components. So far I've tested zend-expressive and zend-code. In 14 seconds it updated all source files and created COPYING.md.

I've ran into another consistency issue.

Expressive:

/**
 * @see       https://github.com/zendframework/zend-expressive for the canonical source repository
 * @copyright https://github.com/zendframework/zend-expressive/blob/master/COPYING.md Copyright
 * @license   https://github.com/zendframework/zend-expressive/blob/master/LICENSE.md New BSD License
 */

zend-code:

/**
 * Zend Framework (http://framework.zend.com/)
 *
 * @link      http://github.com/zendframework/zf2 for the canonical source repository
 * @copyright Copyright (c) 2005-2016 Zend Technologies USA Inc. (http://www.zend.com)
 * @license   http://framework.zend.com/license/new-bsd New BSD License
 */

Specifically the extra text and @see Vs @link. This can be fixed as well, but I need to know what the preferred format should be.

EDIT: According to PSR-5 PHPDoc (if it ever gets released), @link is deprecated. I'll enforce the @see tag.

geerteltink · 2017-04-16T09:02:36Z

src/ZendCodingStandard/Utils/LicenseUtils.php

+
+All rights reserved.
+
+EOF;


Is this text for COPYING.md correct?

I think this can actually be truncated to only the first line. We do not reserve all rights; the LICENSE.md grants the user/consumer a bunch of rights, so long as the copyright is maintained and propagated.

geerteltink · 2017-04-16T09:02:53Z

src/ZendCodingStandard/Utils/LicenseUtils.php

+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+EOF;


Is this text for LICENSE.md correct?

LICENSE.md is correct.

geerteltink · 2017-04-16T09:11:33Z

ruleset.xml

-            <property name="ignoreBlankLines" value="false"/>
-        </properties>
-    </rule>
+    <rule ref="vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml"/>


How to properly include the ruleset in other projects?

<rule ref="vendor/zendframework/zend-coding-standard/ruleset.xml"/>

<rule ref="vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml"/>

The official ruleset.xml file has to be located inside src/ZendCodingStandard otherwise PHP_CodeSniffer doesn't detect the ruleset.
Option 1 is basically a clean wrapper to load option 2. If in the future PHP_CodeSniffer changes something we can just adjust the wrapper without changing all packages. But I don't see this happening any time soon. Option 2 loads the ruleset directly.

Have you tested another repo using this yet?

My primary concerns are:

is resolution of a ref property based on this file, or the file including this file?

what happens when a user decides to use a directory other than vendor? (Composer allows this, believe it or not)

I know that for repositories just switching to zend-coding-standard, we can update them to point to the vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml file; my main concern is repositories already using it.

Have you tested another repo using this yet?

See the updated issue (first post). Tested so far on: zend-expressive, zend-code, zend-expressive-tooling, zend-mvc, zend-http

what happens when a user decides to use a directory other than vendor

This is custom crafted for zend framework components. If someone manages to change the vendor location he needs to change composer.json the component.

Thinking about this, someone might want to use the coding standard for his own project. I could easily add a namespace check so the license will only trigger for zendframework/<end-<component>.

my main concern is repositories already using it.

I haven't tested that. But I think the first travis build or local trigger of phpcs will fail. In that case running phpcbf and a new commit with the changes will fix the build again.

weierophinney

Overall, looks sane and very useful!

The primary concerns are:

s/COPYING/COPYRIGHT/ throughout, as it better implies the purpose.
Uncertainty about how applications already using the coding standard will work against the changes to the ruleset.xml file in the root. If those are a non-concern, we can move forward immediately.
The FileLevelDocBlockSniffer could use a little refactoring (extract methods) for readability and maintainability.

weierophinney · 2017-04-24T15:37:52Z

COPYING.md

@@ -0,0 +1,3 @@
+Copyright (c) 2016-2017, Zend Technologies USA, Inc.


I think we should name this file COPYRIGHT.md instead. COPYING implies that it would contain information on when and how copying code from this repository is allowed, when, in point of fact, that's in the LICENSE.md file. This file only details who owns the copyright.

That will, of course, require a change in the license checking code, IIRC.

weierophinney · 2017-04-24T15:42:13Z

ruleset.xml

-            <property name="ignoreBlankLines" value="false"/>
-        </properties>
-    </rule>
+    <rule ref="vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml"/>


Have you tested another repo using this yet?

My primary concerns are:

is resolution of a ref property based on this file, or the file including this file?

what happens when a user decides to use a directory other than vendor? (Composer allows this, believe it or not)

I know that for repositories just switching to zend-coding-standard, we can update them to point to the vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml file; my main concern is repositories already using it.

weierophinney · 2017-04-24T15:44:08Z

src/ZendCodingStandard/Sniffs/Commenting/FileLevelDocBlockSniff.php

+    public function __construct()
+    {
+        // Get current repo name from composer.json
+        $content = file_get_contents('composer.json');


I wonder if this should be getpwd() . '/composer.json' to be on the safe side? (For those odd cases where folks have changed the include_path to remove . as an entry.)

If getpwd() returns the directory from where phpcs/phpcbf is executed than that is even better.

weierophinney · 2017-04-24T15:45:23Z

src/ZendCodingStandard/Sniffs/Commenting/FileLevelDocBlockSniff.php

+     *
+     * @param File $phpcsFile The PHP_CodeSniffer file where the token was found.
+     * @param int $stackPtr The position in the PHP_CodeSniffer file's token
+     *                      stack where the token was found.


Only indent 4 characters for line continuations; otherwise, we end up with a lot of really short lines for descriptions. It also makes formatting easier when there are changes in parameter types and/or names.

weierophinney · 2017-04-24T15:46:00Z

src/ZendCodingStandard/Sniffs/Commenting/FileLevelDocBlockSniff.php

+    public function process(File $phpcsFile, $stackPtr)
+    {
+        // Skip license and copying file
+        if (in_array(substr($phpcsFile->getFilename(), -10), ['LICENSE.md', 'COPYING.md'])) {


Maybe make constants out of the filenames. Also, this is the first occurence of COPYING.md I've found that will need changes.

weierophinney · 2017-04-24T17:20:37Z

src/ZendCodingStandard/Utils/LicenseUtils.php

+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+EOF;


LICENSE.md is correct.

weierophinney · 2017-04-24T17:20:54Z

src/ZendCodingStandard/Utils/LicenseUtils.php

+     */
+    public static function getCopyrightFile()
+    {
+        return new SplFileInfo('COPYING.md');


s/COPYING/COPYRIGHT/

weierophinney · 2017-04-24T17:21:21Z

src/ZendCodingStandard/ruleset.xml

+    <!-- display progress -->
+    <arg value="p"/>
+    <arg name="colors"/>
+    <file>Sniffs/Files/COPYING.md</file>


s/COPYING/COPYRIGHT/

weierophinney · 2017-04-24T17:21:36Z

src/ZendCodingStandard/ruleset.xml

+    </rule>
+
+    <!-- ZendCodingStandard rules -->
+    <rule ref="ZendCodingStandard.Files.Copying"/>


s/COPYING/COPYRIGHT/

weierophinney · 2017-04-24T17:22:45Z

test/Sniffs/Commenting/TestAsset/FileLevelDocBlockDeprecatedLinkTag.fixed.php

+<?php
+/**
+ * @see       https://github.com/zendframework/zend-coding-standard for the canonical source repository
+ * @copyright https://github.com/zendframework/zend-coding-standard/blob/master/COPYING.md Copyright


s/COPYING/COPYRIGHT/ and so on. I think you can likely grep across the code base for these and fix them.

@weierophinney

per @weierophinney

@weierophinney

per @weierophinney

geerteltink · 2017-05-17T20:57:39Z

Updated to php_codesniffer 3
Simplified detection of LICENSE.md and COPYRIGHT.md (no dummy files needed anymore)
LICENSE.md and COPYRIGHT.md sniffs are now optional.

Example phpcs.xml config file in a component:

<?xml version="1.0"?>
<ruleset name="Zend Framework Coding Standard">
    <!-- Load ruleset -->
    <rule ref="./vendor/zendframework/zend-coding-standard/src/ZendCodingStandard/ruleset.xml"/>

    <!-- Check copyright and license files -->
    <file>./COPYRIGHT.md</file>
    <file>./LICENSE.md</file>
    <!-- Paths to check -->
    <file>./src</file>
    <file>./test</file>

    <!-- Exclude test assets -->
    <exclude-pattern>*/TestAsset/*</exclude-pattern>
</ruleset>

/cc @weierophinney

We still can't use the PHP_CodeSniffer test suite to run tests since it doesn't support PHPUnit 6 yet. Also I think it's nicer to check the exact errors by their id instead of a count to ensure the correct errors are triggered. Some autoload-dev hacks are needed since PHP_CodeSniffer includes its own autoloader and doesn't work with composer out of the box.

There is an issue in PHP_CodeSniffer::Autoload which causes classes to be loaded again during code coverage.

Add license sniff and fixes

weierophinney

Hi, @xtreamwayz !

I've added a comment, but also provided a PR for you (xtreamwayz/zend-coding-standard#2) that performs the method abstractions I've previously requested; tests continue to run fine locally with those changes.

weierophinney · 2017-08-01T15:56:19Z

.travis.yml

+
+notifications:
+  email: false
+  irc:


This line on down can be removed now. 🙌

geerteltink · 2018-04-03T12:38:05Z

Superseded by #4

geerteltink added 2 commits April 12, 2017 17:06

Add sniff + fix for invalid date in COPYING.md and LICENSE.md

841e6bf

weierophinney suggested changes Apr 12, 2017

View reviewed changes

geerteltink added 6 commits April 12, 2017 22:33

Update Travis-CI config

297d314

- Add php 5.6 and 7.1 to the matrix - Use lowest/locked/latest strategy - Enable notifications for IRC and Slack - Generate and upload code coverage per @weierophinney

Add missing composer scripts

e63eef6

Improve consistency, formatting and docblocks

eab40db

per @weierophinney

Use standard phpunit assertions for backwards compatibility

a2159ca

Add missing travis-ci global env's

7575bca

Fix location of ZendCodingStandard ruleset

40d8951

geerteltink added 3 commits April 13, 2017 09:37

Fix code coverage whitelist

189a37b

Enable coveralls

7b5e155

geerteltink added 5 commits April 14, 2017 10:23

Detect and replace deprecated link tag with see tag

82d08bf

Globally exclude TestAssets

8f30be0

Add more tests and a fix for SpacingAfterComment

dc8b879

Enable excluding TestAsset paths again :$

4f309af

Bring readme inline with other components

49c98f6

geerteltink commented Apr 16, 2017

View reviewed changes

weierophinney suggested changes Apr 24, 2017

View reviewed changes

geerteltink and others added 4 commits May 17, 2017 10:18

Update to PHP_CodeSniffer 3.x

71f10ac

Truncate copyright text

5a67fd3

per @weierophinney

s/COPYING/COPYRIGHT/

441ee8a

per @weierophinney

Remove the dummy ruleset and use only the real ruleset

fbd5073

geerteltink and others added 4 commits May 18, 2017 10:22

Run coverage on PHP 7.1

97b0420

Use proper namespace for tests

542d86b

Use only 1 autoloader when running tests

2a8be4c

Disable test coverage

e8e2d85

There is an issue in PHP_CodeSniffer::Autoload which causes classes to be loaded again during code coverage.

michalbundyra mentioned this pull request Jun 8, 2017

[WIP] Extended Zend Framework Coding Standard #1

Closed

34 tasks

froschdesign added the enhancement label Jun 23, 2017

froschdesign added this to the 2.0.0 milestone Jun 23, 2017

weierophinney added a commit that referenced this pull request Aug 1, 2017

Merge pull request #2 from xtreamwayz/feature/license-sniff

6fed723

Add license sniff and fixes

weierophinney suggested changes Aug 1, 2017

View reviewed changes

.travis.yml

notifications:

email: false

irc:

Copy link

Member

weierophinney Aug 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line on down can be removed now. 🙌

michalbundyra mentioned this pull request Oct 11, 2017

Update travis config zendframework/zend-router#31

Merged

michalbundyra mentioned this pull request Apr 1, 2018

License header checks #4

Open

geerteltink closed this Apr 3, 2018

michalbundyra removed this from the 2.0.0 milestone Apr 3, 2018

michalbundyra removed the enhancement label Apr 3, 2018

		@@ -0,0 +1,3 @@
		Copyright (c) 2016-2017, Zend Technologies USA, Inc.

Add license sniff and fixes #2

Add license sniff and fixes #2

Uh oh!

Conversation

geerteltink commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geerteltink commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geerteltink commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

geerteltink commented Apr 12, 2017 •

edited

Loading

geerteltink commented Apr 12, 2017 •

edited

Loading

geerteltink commented Apr 13, 2017 •

edited

Loading

geerteltink commented May 17, 2017 •

edited

Loading